feat: return-item-selection modal on order detail @W-22821837#3886
Conversation
Step 1 of the item-level return flow per the storefront-next 2026-06-17
designs. Renders as a centered Chakra Modal at md+ and a bottom-sheet
Drawer on base (mobile), with returnable items as collapsed rows that
expand into a Quantity stepper + Reason <Select> when checked.
Selection is parent-owned (`selection` + `onSelectionChange` props) so
the wrapper swap on viewport resize doesn't reset the shopper's
progress, and so step 2's review modal (W-22821838) can read the same
payload without prop-drilling.
Reason codes come from `useOmsMetaData().returnReasonCodes`; the entry
with `default: true` is auto-applied when a row is first toggled on.
Loading state renders skeleton rows; failure renders an inline alert
with a Retry button that calls `query.refetch()`.
Quantity uses `useNumberInput` + a styled `Input` (the existing repo
pattern — Chakra's `NumberInput` is not exported from the shared UI),
clamping on blur to the line item's `quantityAvailableToReturn`.
`buildReturnPayload(selection, defaultReasonCode)` shapes the API
request that step 2 will POST: `productItems: [{itemId, quantity, reason?}]`
matching `OmsReturnOrderRequest`. Quantity is serialized as a JS Number
(per oms.yaml `format: double`); reason is omitted when the shopper
kept the OMS default, so the server applies the default per the API
contract (confirmed by Mathias Stödtler in the OMS API thread,
2026-06-10).
Includes 10 Jest tests covering header rendering, selection validation,
default-reason auto-apply, quantity clamping, payload shape (Number
serialization + reason omission), Cancel handler, loading state, and
error+Retry.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Flips the W-22821836 placeholder CTA from disabled to live: clicking it now opens the return-order-modal added in the previous commit. The button's visible label updates from "Start return" to "Return Items" to match the storefront-next designs, but the message id and `data-testid` stay stable so downstream extenders are not broken. Drops the `aria-describedby`/`VisuallyHidden`/`title` "Returns coming soon" plumbing (along with the unused `start_return_disabled_explanation` message) — they were specifically for the placeholder state. Added state on the page: - second `useDisclosure` for the return modal, alongside the existing cancel-order one - `returnSelection` held in `useState`, lifted to the page so the modal can swap between Modal and Drawer on viewport resize without losing shopper progress, and so step 2's review modal (W-22821838) can read the same payload directly - a gated `useProducts` (only fires when modal is open and there is at least one returnable line) merged into `enrichedReturnableItems` so the modal can render "<Name> — <Variation>" lines via `getDisplayVariationValues` `handleCloseReturnModal` resets the local selection on close, mirroring the cancel-order modal's reset semantics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the W-22821836 placeholder assertions (disabled button + "Returns coming soon" accessible description + matching `title`) with checks for the now-enabled state and the renamed visible label. The `data-testid` and message id are stable, so the test still locates the button via `account-order-detail-start-return`. Adds a click-opens-modal smoke test asserting the modal header "Return items from order #..." appears after clicking the trigger. Renamed the describe block from "Start Return CTA (W-22821836)" to "Return Items CTA (W-22821836 / W-22821837)" to reflect that both WIs contribute to the behavior covered here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Ran `npm run extract-default-translations` and `npm run compile-translations` after the string changes: - new `return_order_modal.*` messages (12 entries) for the modal - updated `defaultMessage` for `account_order_detail.button.start_return` from "Start return" to "Return Items" - removed the unused `account_order_detail.button.start_return_disabled_explanation` Tests via `app/utils/test-utils.js` consume the compiled JSON, so the generated files under `app/static/translations/compiled/` must be committed — a regen alone leaves test snapshots stale. Added a one-line CHANGELOG entry under v10.1.0-dev. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Aligns the folder, component, message-id namespace, and data-testid prefix with the W-22821838 plan, which assumes a single modal stack named `return-items-modal/` containing both the selection view (this WI) and the upcoming review view. Keeping the original `return-order-modal/` name would have forced W-22821838 to either move the directory mid-feature or add a same-purpose sibling — neither helpful for reviewers tracking the two WIs together. Changes: - `app/components/return-order-modal/` → `app/components/return-items-modal/` - `ReturnOrderModal` symbol → `ReturnItemsModal` - `data-testid="return-modal-*"` → `data-testid="return-items-modal-*"` (the page-level trigger `account-order-detail-start-return` is unchanged because downstream extenders depend on it via ccExtensibility) - `return_order_modal.*` message ids → `return_items_modal.*` - Regenerated `translations/` and `app/static/translations/compiled/` Tests: all 10 modal tests + 99 orders.test.js tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Eight fixes from the dual Cursor + Claude review pass.
**Ownership guard on Return Items CTA** (order-detail.jsx)
The cancel-order CTA gates on `order.customerInfo.customerId === customerId`
as defense-in-depth against a useOrder response somehow returning an order
the registered shopper does not own; the return CTA was missing the same
check. Added it, plus a covering test that asserts the trigger hides when
ownership doesn't match.
**Rename helper for an unambiguous call site** (constants.js)
`buildReturnPayload` returned the bare productItems array but its JSDoc
referenced `OmsReturnOrderRequest = {productItems: [...]}`, easy to
misread. Renamed to `buildReturnProductItems` so callers wrap it
explicitly: `body: {productItems: buildReturnProductItems(...)}`. Drop-in
for W-22821838's submission step.
**Defensive payload filter** (constants.js)
`Number(row.quantity)` returned `0` for empty strings and `NaN` for
non-numerics. The UI gates on `isSelectionValid`, but the helper is
exported for reuse by W-22821838 and exercised standalone in tests, so
hardened it: rows with non-positive or non-finite quantity are dropped.
Test added.
**Contextual a11y labels** (index.jsx, constants.js)
The +/- stepper buttons rendered as bare "−"/"+", and Quantity / Reason
form controls labelled identically across rows so screen-reader users
couldn't tell rows apart. Added `messages.quantityFor`, `reasonFor`,
`quantityIncrement`, `quantityDecrement` and threaded `productName` down
to QuantityField. Visible FormLabels stay short ("Quantity", "Reason");
contextual labels live on `aria-label`.
**`role="status"` + visible-or-hidden loading announcement** (index.jsx)
Skeleton block now wrapped in `role="status"` with a `VisuallyHidden`
"Loading return reasons…" message (the `loadingReasons` entry in the
catalog was previously unused).
**`useId()` for the disabled-hint id** (index.jsx)
Replaced the hardcoded `'return-items-modal-review-disabled-hint'` string
with `useId()` so two modal instances on the same page wouldn't collide.
**Drop the no-op ICU plurals** (constants.js)
`availableToReturn` and `itemCheckboxLabel` declared `{count, plural,
one {…} other {…}}` with identical text in both branches — translators
would flag this. Differentiated the strings ("# unit" vs "# units").
**Backfill regression test** (index.test.js)
Added a test for the `didBackfillRef` retro-fill path: a row pre-checked
without a reasonCode (e.g. selection rehydrated from URL state in a
future WI) gets the OMS default applied on mount.
Tests: 112 pass (12 modal + 100 orders).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Git2Gus App is installed but the |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
sf-jie-dai
added
the
skip changelog
Mirrors the W-22821836 cleanup (e382e89) — reviewers prefer the modal work be tracked via the GUS WI rather than a separate CHANGELOG line. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI runs `npm run build-translations` which chains `extract-default-translations` + `compile-translations` + `compile-translations:pseudo`, then the smoke-test job fails the build if `git diff --exit-code` finds uncommitted changes. I had only run the first two locally, leaving `en-XA.json` (the pseudo locale) stale relative to the renamed `return_items_modal.*` message ids and the new contextual a11y labels (`quantity_for`, `reason_for`, `quantity_increment`, `quantity_decrement`). Regenerated via `npm run build-translations`. No code changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| }, | ||
| reasonsError: { | ||
| defaultMessage: 'We could not load the return reasons. Please try again.', | ||
| id: 'return_items_modal.text.reasons_error' |
There was a problem hiding this comment.
@sf-jie-dai Did you get messages for various error scenarios from Sab or are we supposed to show a default message for all errors?
There was a problem hiding this comment.
I have a W-22821839 especially for error states but I could ask Sab now
There was a problem hiding this comment.
Reached out here
but an error related to SCAPI. I think a generic one should be okay since it shouldn't happen often
|
@sf-jie-dai 🐛 Bug: Stale closure in
|
Switch updateRow and handleToggle to functional setState updaters so two checkbox toggles dispatched in the same React batch both observe the latest selection. Add a regression test that fires two toggles inside a single act() and asserts both rows end up checked. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop the inline QuantityField in favor of the shared QuantityPicker component, which already provides mobile-friendly select-on-focus, keyboard activation on the +/- buttons, and i18n strings for the increment/decrement aria-labels. Remove the now-unused quantity_for, quantity_increment, and quantity_decrement message ids from the return-items-modal catalog and regenerate en-US, en-GB, and en-XA translations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The OMS-expanded order already carries productName, variationAttributes, and variationValues on each line item, so the gated useProducts fetch added nothing the modal could not derive locally. Removing it also avoids the SCAPI /products 24-id ceiling that the call would have hit on large orders. Pass returnableItems straight through to ReturnItemsModal — the formatVariationSummary helper already reads variationAttributes / variationValues directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wrap ReturnableItemRow in React.memo so it skips re-rendering when neither its item nor its row state changed. Move the inline arrow wrappers (onToggle / onQuantityChange / onReasonChange) into useCallback inside the row itself, and pass the parent's stable handlers straight through, so the per-row props no longer change on every keystroke. This compounds with the FIX-1 functional-updater change: now the parent handlers depend only on [onSelectionChange, defaultReasonCode] and the child handlers depend only on [parent handler, itemId], so unrelated rows do not re-render while one row's quantity is being edited. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Relocate the helper out of return-items-modal/constants.js (which now holds only intl message definitions) into the existing return-utils.js alongside getReturnableItems. Update the modal import and migrate the helper-only unit tests to return-utils.test.js so the modal test file stays focused on UI behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@sf-madhuri-uppu Thanks for the review — all 5 findings addressed in the latest push. Summary
Scope note on error handlingThe full per-error-code mapping (matching what your storefront-next PR 1911 layered in Lint and the full |
…eturnItemSelectionModal Resolves conflicts with #3883 (cancel-modal OMS-metadata polish) and #3884 (cancel error scenarios) merged into the base branch: - orders.test.js: adopt base's `waitFor` import (used by the new cancel error tests) and align the return-eligible order mock to the new `useCustomerId: () => 'testCustomerId'` mock that base introduced (previously the test used the JWT-derived id from test-utils). - order-detail.jsx: prettier-fold the cancel-error description line and the `isDisabled` predicate after the cancelTerminal addition. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| <Button | ||
| colorScheme="blue" | ||
| onClick={handleReview} | ||
| isDisabled={!reviewEnabled} |
There was a problem hiding this comment.
The WI asks for the disabled state to be announced via aria-disabled. Chakra's isDisabled renders the native disabled attribute, which removes the button from the tab order — so a keyboard/SR user can't focus it and the aria-describedby hint below never gets announced. Consider aria-disabled (keep it focusable, guard the onClick) or Chakra's focusable-when-disabled pattern. Note the aria-describedby test passes on attribute-presence but doesn't verify the hint is reachable.
There was a problem hiding this comment.
Good catch — fixed in 6e3174f. Switched the Review button from isDisabled to aria-disabled={!reviewEnabled} so it stays in the tab order while invalid, and handleReview now no-ops when invalid (focusable means clickable). Chakra's _disabled styling still applies since the pseudo matches [aria-disabled=true]. Updated the test to assert the aria-describedby hint is actually reachable (resolves to a node in the document carrying the explanation), not just present.
| <Text fontSize="lg" fontWeight="bold"> | ||
| <FormattedMessage {...messages.title} values={{orderNo: order?.orderNo}} /> | ||
| </Text> | ||
| <Text fontSize="sm" color="gray.600" fontWeight="normal"> |
There was a problem hiding this comment.
The WI wants the modal described by the subhead (aria-describedby). Chakra auto-wires aria-labelledby to the header for the title, but the subhead isn't associated as the description. Giving this Text an id and setting aria-describedby on the ModalContent/DrawerContent would satisfy it.
There was a problem hiding this comment.
Fixed in 6e3174f. Heads-up on the mechanism: setting aria-describedby directly on ModalContent/DrawerContent is silently overridden by Chakra 2.7's getDialogProps, which unconditionally forces the dialog's aria-describedby to the ModalBody id whenever a body is mounted (verified by probing the rendered DOM). So instead I moved the subhead to lead the body — Chakra's auto aria-describedby now points at it, and the title stays in the header for aria-labelledby. This mirrors the cancel-order-modal pattern.
| <DrawerOverlay /> | ||
| <DrawerContent data-testid="return-items-modal-drawer"> | ||
| <DrawerHeader pb={1}>{header}</DrawerHeader> | ||
| <DrawerCloseButton /> |
There was a problem hiding this comment.
The WI specifies the mobile bottom-sheet shows the standard drag handle with no close X, but this renders a DrawerCloseButton. Chakra's Drawer has no built-in drag handle (and the product-list bottom Drawer also uses DrawerCloseButton), so — intended deviation, or should this drop the X and add a handle?
There was a problem hiding this comment.
I checked the design spec (storefront-next#2038, the 2026-06-18 rewrite that matches the latest WI-3). It doesn't specify a drag handle anywhere, and the WI-3 section says to reuse the cancel-order-modal pattern with ModalCloseButton. So I'm keeping DrawerCloseButton here — it's spec-aligned and consistent with the existing product-list bottom Drawer. The "standard drag handle (no close X)" line in the GUS WI text looks stale relative to the spec; happy to revisit if design confirms otherwise.
| /* | ||
| * Copyright (c) 2026, salesforce.com, inc. | ||
| * All rights reserved. | ||
| * SPDX-License-Identifier: BSD-3-Clause | ||
| * For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause | ||
| */ | ||
| import React, {useState} from 'react' | ||
| import PropTypes from 'prop-types' | ||
| import {act, fireEvent, screen, waitFor, within} from '@testing-library/react' | ||
| import userEvent from '@testing-library/user-event' | ||
| import {renderWithProviders} from '@salesforce/retail-react-app/app/utils/test-utils' | ||
| import ReturnItemsModal from '@salesforce/retail-react-app/app/components/return-items-modal' | ||
|
|
||
| let mockOmsMetaData = { | ||
| data: { | ||
| cancelReasonCodes: [], | ||
| returnReasonCodes: [ | ||
| {reason: 'Wrong size', default: true}, | ||
| {reason: 'Defect', default: false}, | ||
| {reason: 'Changed my mind', default: false} | ||
| ] | ||
| }, | ||
| isLoading: false, | ||
| isError: false, | ||
| refetch: jest.fn() | ||
| } | ||
| jest.mock('@salesforce/commerce-sdk-react', () => { | ||
| const actual = jest.requireActual('@salesforce/commerce-sdk-react') | ||
| return { | ||
| ...actual, | ||
| useOmsMetaData: () => mockOmsMetaData | ||
| } | ||
| }) | ||
|
|
||
| const baseOrder = { | ||
| orderNo: '00123456', | ||
| productItems: [ | ||
| { | ||
| itemId: 'item-1', | ||
| productId: 'prod-1', | ||
| productName: 'Cotton Crew T-Shirt', | ||
| quantity: 2, | ||
| omsData: {quantityAvailableToReturn: 2}, | ||
| variationAttributes: [ | ||
| {id: 'color', name: 'Color', values: [{value: 'BLACK', name: 'Black'}]}, | ||
| {id: 'size', name: 'Size', values: [{value: 'M', name: 'M'}]} | ||
| ], | ||
| variationValues: {color: 'BLACK', size: 'M'} | ||
| }, | ||
| { | ||
| itemId: 'item-2', | ||
| productId: 'prod-2', | ||
| productName: 'Slim Fit Chino Pants', | ||
| quantity: 1, | ||
| omsData: {quantityAvailableToReturn: 1} | ||
| } | ||
| ] | ||
| } | ||
|
|
||
| const Harness = ({onReview = jest.fn(), onClose = jest.fn(), initialSelection = {}} = {}) => { | ||
| const [selection, setSelection] = useState(initialSelection) | ||
| return ( | ||
| <ReturnItemsModal | ||
| isOpen={true} | ||
| onClose={onClose} | ||
| order={baseOrder} | ||
| returnableItems={baseOrder.productItems} | ||
| selection={selection} | ||
| onSelectionChange={setSelection} | ||
| onReview={onReview} | ||
| /> | ||
| ) | ||
| } | ||
| Harness.propTypes = { | ||
| onReview: PropTypes.func, | ||
| onClose: PropTypes.func, | ||
| initialSelection: PropTypes.object | ||
| } | ||
|
|
||
| afterEach(() => { | ||
| mockOmsMetaData = { | ||
| data: { | ||
| cancelReasonCodes: [], | ||
| returnReasonCodes: [ | ||
| {reason: 'Wrong size', default: true}, | ||
| {reason: 'Defect', default: false}, | ||
| {reason: 'Changed my mind', default: false} | ||
| ] | ||
| }, | ||
| isLoading: false, | ||
| isError: false, | ||
| refetch: jest.fn() | ||
| } | ||
| jest.clearAllMocks() | ||
| }) | ||
|
|
||
| test('renders header with order number and a row per returnable item', async () => { | ||
| renderWithProviders(<Harness />) | ||
| expect(await screen.findByText(/return items from order #00123456/i)).toBeInTheDocument() | ||
| expect(screen.getByText(/select the items you want to return/i)).toBeInTheDocument() | ||
| expect(screen.getAllByTestId('return-items-modal-item-row')).toHaveLength(2) | ||
| expect(screen.getByText(/cotton crew t-shirt/i)).toBeInTheDocument() | ||
| expect(screen.getByText(/slim fit chino pants/i)).toBeInTheDocument() | ||
| }) | ||
|
|
||
| test('Review return is disabled until at least one valid row is selected', async () => { | ||
| const user = userEvent.setup() | ||
| renderWithProviders(<Harness />) | ||
| const reviewButton = screen.getByTestId('return-items-modal-review') | ||
| expect(reviewButton).toBeDisabled() | ||
| expect(reviewButton).toHaveAttribute('aria-describedby') | ||
|
|
||
| const checkboxes = screen.getAllByRole('checkbox') | ||
| await user.click(checkboxes[0]) | ||
| expect(reviewButton).toBeEnabled() | ||
| expect(reviewButton).not.toHaveAttribute('aria-describedby') | ||
| }) | ||
|
|
||
| test('toggling a row expands it and pre-selects the OMS default reason', async () => { | ||
| const user = userEvent.setup() | ||
| renderWithProviders(<Harness />) | ||
| const checkboxes = screen.getAllByRole('checkbox') | ||
| await user.click(checkboxes[0]) | ||
|
|
||
| const row = screen.getAllByTestId('return-items-modal-item-row')[0] | ||
| // Reason carries a per-row aria-label (so screen readers can distinguish | ||
| // the dropdowns). Quantity reuses the shared QuantityPicker which sets | ||
| // aria-label="Quantity"; we scope to the row + the input element to | ||
| // disambiguate from the +/- buttons. | ||
| expect(within(row).getByLabelText(/reason for /i, {selector: 'select'})).toHaveValue( | ||
| 'Wrong size' | ||
| ) | ||
| expect(within(row).getByLabelText(/^quantity$/i, {selector: 'input'})).toHaveValue('1') | ||
| }) | ||
|
|
||
| test('quantity field clamps to the available-to-return ceiling', async () => { | ||
| const user = userEvent.setup() | ||
| renderWithProviders(<Harness />) | ||
| const checkboxes = screen.getAllByRole('checkbox') | ||
| await user.click(checkboxes[0]) // item-1 has max 2 | ||
|
|
||
| const row = screen.getAllByTestId('return-items-modal-item-row')[0] | ||
| const qty = within(row).getByLabelText(/^quantity$/i, {selector: 'input'}) | ||
| // Set value directly then blur — Chakra's useNumberInput clamps on blur, | ||
| // and userEvent.clear() doesn't propagate to a controlled NumberInput | ||
| // because the hook rejects empty intermediate values. | ||
| fireEvent.change(qty, {target: {value: '99'}}) | ||
| fireEvent.blur(qty) | ||
| expect(qty).toHaveValue('2') | ||
| }) | ||
|
|
||
| test('Cancel calls onClose', async () => { | ||
| const user = userEvent.setup() | ||
| const onClose = jest.fn() | ||
| renderWithProviders(<Harness onClose={onClose} />) | ||
| await user.click(screen.getByTestId('return-items-modal-cancel')) | ||
| expect(onClose).toHaveBeenCalledTimes(1) | ||
| }) | ||
|
|
||
| test('clicking Review return forwards a properly shaped payload', async () => { | ||
| const user = userEvent.setup() | ||
| const onReview = jest.fn() | ||
| renderWithProviders(<Harness onReview={onReview} />) | ||
|
|
||
| const checkboxes = screen.getAllByRole('checkbox') | ||
| await user.click(checkboxes[1]) // item-2: max 1, should default-reason | ||
|
|
||
| // Change reason away from default so it gets serialized | ||
| const reason = within(screen.getAllByTestId('return-items-modal-item-row')[1]).getByLabelText( | ||
| /reason for /i, | ||
| {selector: 'select'} | ||
| ) | ||
| await user.selectOptions(reason, 'Defect') | ||
|
|
||
| await user.click(screen.getByTestId('return-items-modal-review')) | ||
| expect(onReview).toHaveBeenCalledWith([{itemId: 'item-2', quantity: 1, reason: 'Defect'}]) | ||
| }) | ||
|
|
||
| test('renders skeleton placeholders while OMS metadata is loading', () => { | ||
| mockOmsMetaData = {data: undefined, isLoading: true, isError: false, refetch: jest.fn()} | ||
| renderWithProviders(<Harness />) | ||
| expect(screen.getByTestId('return-items-modal-loading')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| test('backfills the OMS default reason on already-checked rows when metadata is available', async () => { | ||
| // Models the case where the parent has a checked row whose reasonCode | ||
| // was never set (e.g. selection rehydrated from URL state in a future | ||
| // WI, or initial open where the user clicked the row before reasons | ||
| // resolved). The modal's mount-time backfill effect must apply the | ||
| // OMS default so the row is valid without forcing a re-pick. | ||
| const initial = {'item-1': {checked: true, quantity: 1, reasonCode: undefined}} | ||
|
|
||
| renderWithProviders(<Harness initialSelection={initial} />) | ||
|
|
||
| await waitFor(() => expect(screen.getByTestId('return-items-modal-review')).toBeEnabled()) | ||
| const row = screen.getAllByTestId('return-items-modal-item-row')[0] | ||
| expect(within(row).getByLabelText(/reason for /i, {selector: 'select'})).toHaveValue( | ||
| 'Wrong size' | ||
| ) | ||
| }) | ||
|
|
||
| test('two toggles in the same React batch both stick (no stale closure)', async () => { | ||
| // Regression: updateRow used to close over `selection`, so two checkbox | ||
| // toggles dispatched in a single render cycle each spread the same stale | ||
| // object — only the second won, silently dropping the first row. | ||
| renderWithProviders(<Harness />) | ||
| const [first, second] = screen.getAllByRole('checkbox') | ||
| act(() => { | ||
| fireEvent.click(first) | ||
| fireEvent.click(second) | ||
| }) | ||
| await waitFor(() => expect(first).toBeChecked()) | ||
| expect(second).toBeChecked() | ||
| }) | ||
|
|
||
| test('renders an error alert + Retry when OMS metadata fails', async () => { | ||
| const user = userEvent.setup() | ||
| const refetch = jest.fn() | ||
| mockOmsMetaData = {data: undefined, isLoading: false, isError: true, refetch} | ||
| renderWithProviders(<Harness />) | ||
| expect(screen.getByTestId('return-items-modal-error')).toBeInTheDocument() | ||
| await user.click(screen.getByTestId('return-items-modal-retry')) | ||
| expect(refetch).toHaveBeenCalledTimes(1) | ||
| }) |
There was a problem hiding this comment.
The WI lists "focus return on close" and "axe-core baseline clean" as test ACs; neither is here (and there's no jest-axe in the repo yet). The suite also only renders the desktop Modal branch — the mobile Drawer isn't exercised. Worth a focus-return assertion + an axe baseline (and ideally a Drawer-branch render).
There was a problem hiding this comment.
Added in 6e3174f: a focus-return-on-close assertion and a mobile Drawer-branch render test (mocking useBreakpointValue). Holding off on the axe-core baseline for now — it needs a new jest-axe devDependency, and axe coverage is explicitly scoped to WI-6 (W-22821844, "A11y, i18n, responsive polish"), so I'll introduce the dep + baseline there rather than here. Suite is green at 12 tests.
sf-shikhar-prasoon
left a comment
sf-shikhar-prasoon
left a comment
There was a problem hiding this comment.
Looks solid overall — the prior review's items (functional updaters, QuantityPicker reuse, dropped useProducts, memoization, helper relocation) are all in, the merge with the cancel PRs is clean, and the modal tests are thorough. A few accessibility points against the WI's a11y ACs, plus two test-coverage gaps the WI calls out. None blocking.
a11y (vs the WI's explicit ACs):
isDisabledvsaria-disabledon "Review return": Chakra'sisDisabledrenders the nativedisabledattribute, which drops the button from the tab order — so thearia-describedbyhint can't actually be reached/announced by a keyboard or screen-reader user. The WI asks foraria-disabledspecifically so the control stays focusable and the hint is announced. (The test asserting the attribute exists passes but doesn't prove it's reachable.)- Subhead isn't wired as the modal's description: Chakra auto-wires
aria-labelledbyto the header (title), but the subhead isn't associated viaaria-describedbyon the Modal/Drawer. The WI asks for "described by subhead." Anidon the subhead +aria-describedbyon the content would close it. - Mobile bottom-sheet renders a close-X: the WI specifies a drag handle with no close X on mobile, but the Drawer renders
DrawerCloseButton. (Noting Chakra's Drawer has no built-in drag handle, and the product-list bottom Drawer also usesDrawerCloseButton— so flag whether this is an intended deviation.)
tests (WI lists both as ACs): no "focus return on close" test and no axe-core baseline (the repo has no axe testing today). The suite also only exercises the desktop Modal branch — the mobile Drawer is untested.
Non-blocking, for the W-22821838 reuse: buildReturnProductItems is exported for the submit step but doesn't clamp quantity to quantityAvailableToReturn (only the UI does), and omits the reason whenever reasonCode === defaultReasonCode — which sends an explicit reason for everything if defaultReasonCode is ever undefined. Both are unreachable through this PR's UI, but worth hardening since the helper is the shared seam. Minor: row max uses ?? 1 while isSelectionValid uses ?? 0 (dead branch given the getReturnableItems filter, but worth matching).
…21837 Address review feedback on the return-item-selection modal: - Review button: use aria-disabled instead of isDisabled so the button stays in the tab order while invalid, letting keyboard/SR users reach the aria-describedby hint that explains why it's disabled. Chakra's _disabled styling still applies (pseudo matches [aria-disabled=true]); handleReview no-ops when invalid. - Subhead is now the modal's accessible description: moved it to lead the ModalBody/DrawerBody so Chakra's auto aria-describedby (-> body id) points at it. Setting aria-describedby on ModalContent is silently overridden by Chakra's getDialogProps, so this mirrors cancel-order-modal. - Tests: assert the disabled hint is reachable (not just present), add a focus-return-on-close assertion, and exercise the mobile Drawer branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sf-jie-dai
merged commit 1425fd8
into
feature/264-order-management
4 participants
Context
Phase 2 of the PWA Kit item-level return flow for registered shoppers. Today's
Return Itemsbutton on the order-detail page is a disabled W-22821836 placeholder with a "Returns coming soon" hint. This PR makes it functional: clicking it opens a Chakra modal where the shopper picks which items to return, sets quantity, and chooses a reason fromgetOmsMetaData. The follow-up review/submit step lives in W-22821838.Summary
app/components/return-items-modal/— centered Modal atmd+, bottom-sheet Drawer onbase. Per-item bordered rows collapse to a checkbox + name + "Up to N units available to return"; expand to a quantity stepper (useNumberInput+Input, clamped toquantityAvailableToReturn) and a reason<Select>populated fromuseOmsMetaData().returnReasonCodeswith thedefault: trueentry pre-applied.order-detail.jsx, not the modal, so the wrapper swap on viewport resize doesn't reset progress and so step 2 (W-22821838) can read the same payload directly. A defensiveuseEffectretro-fills the OMS default reason on rows that were checked before metadata resolved.Return ItemsCTA flipped from disabled to live; visible label updated fromStart returntoReturn Items(message id anddata-testidkept stable so downstream extenders aren't broken). The CTA gates on customer ownership of the order (order.customerInfo.customerId === customerId) plus the per-itemquantityAvailableToReturnsignal — same defense-in-depth as the cancel-order CTA.buildReturnProductItems(selection, defaultReasonCode)exported for reuse by W-22821838's submit step. Returns theproductItemsarray shape fromOmsReturnOrderRequest:quantityserialized as a JS Number peroms.yaml'sformat: double,reasonomitted when the shopper kept the OMS default so the server applies it. Drops malformed rows (non-finite or non-positive quantity) defensively.aria-labels including the product name; loading state wrapped inrole="status"with a visually-hidden announcement; the disabledReview returnbutton uses auseId()-generatedaria-describedbyhint explaining why.return items button:
return modal:
Test plan
pnpm test— 12 modal tests + 100 page-level tests pass (covers selection validation, default-reason apply, quantity clamp, payload shape, ownership-mismatch hide, click-opens-modal, loading + error UX, retro-fill on metadata resolve)zymc-002sandbox (build26.7.0.166-master, SOM-wired): order detail with at least one shipped item shows the Return Items button; clicking opens the modal, item rows render with variation summary, default reason pre-selected, quantity clamps to max, Review return enables only when validmdbreakpoint while modal is open — selection survives the Modal ↔ Drawer wrapper swap